Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CBTCU v1.9 #15

Merged
merged 2 commits into from
May 3, 2024
Merged

CBTCU v1.9 #15

merged 2 commits into from
May 3, 2024

Conversation

tackandr
Copy link
Contributor

@tackandr tackandr commented Apr 30, 2024

Implementation of latest version of CB-TCU cloud. Smartmet Macro below:
https://wiki.fmi.fi/pages/viewpage.action?spaceKey=PROJEKTIT&title=Cb-TCu-top_FL

@tackandr tackandr requested review from mpartio and KorpinenA April 30, 2024 10:02
@@ -10,25 +10,33 @@ local MU = level(HPLevelType.kMaximumThetaE,0)
local HL = level(HPLevelType.kHeightLayer,500,0)
local HG = level(HPLevelType.kHeight,0)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"local" could be used for these variables

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "local" actually do?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it declares the variable as a local variable instead of global, as you may have guessed :D

Basically it is the way lua recommends to be done: always use "local" unless you have a reason not to. It also has some performance implications as fetching a variable from global scope is more expensive. I have not benchmarked this though and I guess it's rather small, unless running on a very tight loop.

return
end

CBlimit = 2000 --required vertical thickness [degrees C] to consider a CB (tweak this..!)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment for these variables is a leftover from earlier commit?

Also in smarttool macro TCulimit is 1500 (not 1000).

res[i] = FlightLevel_(EL500[i]*100)
if (EL500[i] - LCL500[i] > TCUlimit) then
--we don't use vertical search for flight level of EL500 but calculate directly from EL500 pressure
res[i] = FlightLevel_(pEL500[i] * 100)
Copy link
Member

@mpartio mpartio May 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is clever 👍

In thing that caught my eye is that the smarttool function call is:

vertfl_findh(par3_meps,0,TopLim,EL500,1)

The search is capped to TopLim which is set to FL 650. Should this also be checked here, ie.

if res[i] > 650 then 
  res[i] = Missing
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is just there because smarttool function requires some value and FL650 is so high that EL500 will always be below. At least in our latitudes. And if there is 20km tall convection I think it's better to put it as it is than to turn it into missing.

@@ -64,10 +74,11 @@ for i=1, #EL500 do
res[i] = Missing

--TCU
if ((Tbase[i]-Ttop[i]>TCUlimit) and (NL[i]>0) and (CIN500[i]>CINlimTCU)) then
res[i] = FlightLevel_(EL500[i]*100)
if (EL500[i] - LCL500[i] > TCUlimit) then
Copy link
Member

@mpartio mpartio May 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In smarttool the if condition is:

if ((EL500-LCL500>TCulimit) and (cl_meps>0) and (CIN500>CINlimTCu))
The two latter boolean conditions are missing

--Limit top value
if (CAPE500[i]>math.exp(1)) then
if (CAPE500[i] > math.exp(1)) then
--Add for overshooting top based on CAPE
res[i] = -(res[i] + CAPE500[i] / (math.log(CAPE500[i]) * 10))
Copy link
Member

@mpartio mpartio May 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In smarttool the last term (10) has a division operator, not multiplication. I tried to think if you are doing some kind of mathematical trick here but I couldn't get it :D

I don't understand why Simo is using exp(1) here, it's just a constant number.

Copy link
Contributor Author

@tackandr tackandr May 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put () around it which is the mathematical trick you missed. On a high nerd level I think multiplication is a cheaper operation than division.

exp(1) is used to generate the constant e that LUA doesn't provide. This was needed because we had some problems where CAPE was small and then the logarithm function goes to 0 for ln(1) and division by 0 is bad as well as for values below 1 it gives negative numbers that potentially turns the sign of the result around. Which by the logic of this macro turns CB into TCU. Probably it's not wise to have two things in one parameter and we should output CB and TCU as two separate fields but that's what it is now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just wondering why e(1), does it have some special significance. Why doesn't he just declare "const CAPElimit = 2.78".

I agree it's a bit weird that this parameter has different interpretation depending on the sign of the data value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think I'll change it to that CAPElimit version then.

@mpartio
Copy link
Member

mpartio commented May 2, 2024

Following this script was quite easy.

I also liked that I could see the whole logic inside for loop on my screen without having to scroll up and down. Often in the internet you see people arguing for "clean code" which seems to mean that everything should be split to as small functions as possible, which is maybe better for code reuse but it makes reading the code hard.

@KorpinenA
Copy link

I checked the code through and nothing obvious didn't pop out. I'm not that familiar with the code though. But the code and calculations seems to be well written and the code itself is very easy to understand. So merging is ok.

@tackandr tackandr merged commit af90d90 into master May 3, 2024
@tackandr tackandr deleted the STU-24065 branch May 3, 2024 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants